Stabilize ExceptionEventData #6795

vasantteja · 2024-10-16T01:38:07Z

This Change

I promoted both the interface and implementation to data package adhering to the package structure inside the internal implementation. Let me know your thoughts. Sorry for opening another pr when we have another pr. This resolves 6630

jkwatson · 2024-10-16T01:44:56Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ExceptionEventData.java

@@ -18,13 +17,15 @@
 public interface ExceptionEventData extends EventData {

  /**
-   * Returns a new immutable {@link ExceptionEventData}.
+   * Returns a new immutable {@link io.opentelemetry.sdk.trace.data.ExceptionEventData}.


this shouldn't require being fully qualified, I don't think. I'd revert the fully-qualified classname changes here.

Dang it, sorry I will just use the class name and remove the fully-qualified classname.

jkwatson · 2024-10-16T01:45:14Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ImmutableExceptionEventData.java

 @AutoValue
 @Immutable
-abstract class ImmutableExceptionEventData implements ExceptionEventData {
+abstract class ImmutableExceptionEventData
+    implements io.opentelemetry.sdk.trace.data.ExceptionEventData {


this should be importable, and not need to be fully-qualified

codecov · 2024-10-16T01:49:20Z

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.49%. Comparing base (b1cd30e) to head (18c2578).
Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
.../main/java/io/opentelemetry/sdk/trace/SdkSpan.java	92.30%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6795      +/-   ##
============================================
+ Coverage     90.12%   90.49%   +0.37%     
- Complexity     6546     6598      +52     
============================================
  Files           728      731       +3     
  Lines         19702    19735      +33     
  Branches       1935     1938       +3     
============================================
+ Hits          17756    17859     +103     
+ Misses         1347     1283      -64     
+ Partials        599      593       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…e a little for removing misleading comments from copy paste.

jkwatson

Thanks!

jack-berg

Few discussion points

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ExceptionEventData.java

jack-berg · 2024-10-21T15:21:55Z

docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt

+	+++  NEW INTERFACE: io.opentelemetry.sdk.trace.data.EventData
+	+++  NEW SUPERCLASS: java.lang.Object
+	+++  NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.trace.data.ExceptionEventData create(io.opentelemetry.sdk.trace.SpanLimits, long, java.lang.Throwable, io.opentelemetry.api.common.Attributes)


@jkwatson its odd for the create method to accept SpanLimits as an argument. Normally the application of limits is handled in SDK internals. We can see this pattern play out in the EventData.create contract:

static EventData create( long epochNanos, String name, Attributes attributes, int totalAttributeCount)

The attributes are provided with an additional totalAttributeCount indicating how many attributes were originally present.

The reason we do it differently for ExceptionEventData appears to be because ImmutableExceptionEvent#getAttributes() doesn't compute the attributes until it is called (and then it memoizes them), preventing additional allocations on the hot path.

Not sure there's a way to avoid SpanLimits being passed as an argument if we stick with that argument.

jack-berg · 2024-10-21T15:27:57Z

docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt

+	+++  NEW SUPERCLASS: java.lang.Object
+	+++  NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.trace.data.ExceptionEventData create(io.opentelemetry.sdk.trace.SpanLimits, long, java.lang.Throwable, io.opentelemetry.api.common.Attributes)
+	+++  NEW METHOD: PUBLIC(+) ABSTRACT(+) io.opentelemetry.api.common.Attributes getAdditionalAttributes()


Upon closer inspection, I find this method strange. The resulting ExceptionEventData interface has the following methods:

getAttributes(): default implementation returns getAdditionalAttributes() provided by user + exception.* attributes following semconv.

getAdditionalAttributes(): default implementation returns only additional attributes provided by the user.

Seems odd to have a special accessor for getAdditionalAttributes(). I assume that consumers of this want to do something like: if(event instanceOf ExceptionEventData) serialize(((ExceptionEventData) event).getException())

If they want o split out the exception attributes from additional attributes, can do so with getAttributes().toBuilder().removeIf(attributeKey -> attributeKey.getKey().startsWith("exception")).build().

Do we really need the special getAdditionalAttributes() accessor?

hmm. good eye on double checking the original API for this. Maybe we're not quite ready to stabilize, if we're unsure of the final API? I agree the creational patterns for this are currently strange and not user-friendly. 🤔

I think we can still stabilize, but cut out the getAdditionalAttributes() method. The point I make here about SpanLimits as a constructor argument is an implementation detail. We can remove it as a parameter with the only difference being that we compute the exception.* attributes on the calling thread instead of on a background thread, which seems pretty reasonable / expected.

If you agree, I can either push some commits to this PR, or provide some more complete guidance to @vasantteja on how to proceed.

I'm totally ok with your suggestion

@jack-berg I understand that we should remove getAdditionalAttributes() and SpanLimits from the implementation. Your suggestion to make this similar to EventData makes sense to me. The reason behind this is that the caller can retrieve the additional attributes using this call getAttributes().toBuilder().removeIf(attributeKey -> attributeKey.getKey().startsWith("exception")).build();. Did I get this right, or am I missing anything? If yes, I can make the changes suggested.

You've got it.

The exception.* attributes will need to be computed in SdkSpan, rather than in ImmutableExceptionEventData.

@jack-berg I finally wrapped my head around this and have an implementation. I cut out the attribute computation in the ImmutableExceptionEventData and I moved them to SdkSpan. I added a little bit of logic to shrink the exception messages if maxLength attribute is turned on in SpanLimits. Let me know if this aligns with your thinking.

Co-authored-by: jack-berg <[email protected]>

things change

…ata to align with EventData.

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ImmutableExceptionEventData.java

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

…bleExceptionEventData.java Co-authored-by: jack-berg <[email protected]>

Co-authored-by: jack-berg <[email protected]>

…thods

jack-berg · 2024-10-29T14:09:19Z

I pushed a small commit to capture the total number of attributes added (in case any were filtered to conform to SpanLimits), and to remove unused methods. Looks good to merge to me!

trask · 2024-10-29T18:28:55Z

docs/apidiffs/current_vs_latest/opentelemetry-sdk-trace.txt

+	+++  CLASS FILE FORMAT VERSION: 52.0 <- n.a.
+	+++  NEW INTERFACE: io.opentelemetry.sdk.trace.data.EventData
+	+++  NEW SUPERCLASS: java.lang.Object
+	+++  NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.trace.data.ExceptionEventData create(long, java.lang.Throwable, io.opentelemetry.api.common.Attributes, int)
+	+++  NEW METHOD: PUBLIC(+) ABSTRACT(+) java.lang.Throwable getException()


vasantteja added 2 commits October 15, 2024 08:54

wip: stabilizing event exception data.

219ae6f

polish: adding api diffs.

e7c0a03

vasantteja requested a review from a team as a code owner October 16, 2024 01:38

jkwatson reviewed Oct 16, 2024

View reviewed changes

vasantteja added 2 commits October 15, 2024 21:59

polish: removing the fully qualified class names and refining the cod…

015a707

…e a little for removing misleading comments from copy paste.

polish: adding a missing comment.

b6e5816

jkwatson previously approved these changes Oct 16, 2024

View reviewed changes

jack-berg reviewed Oct 21, 2024

View reviewed changes

vasantteja and others added 2 commits October 21, 2024 21:33

polish: Update the comment around the class.

9700e01

Co-authored-by: jack-berg <[email protected]>

polish: applying spotless checks.

661fc17

jkwatson self-requested a review October 22, 2024 20:10

vasantteja added 2 commits October 27, 2024 17:34

polish: removing additional attributes and converting ExceptionEventD…

b89292a

…ata to align with EventData.

polish: removing the unwanted initialization for attributes.

7cc7162

jkwatson reviewed Oct 27, 2024

View reviewed changes

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java Outdated Show resolved Hide resolved

polish: static instantiation.

e5e686c

jack-berg reviewed Oct 28, 2024

View reviewed changes

vasantteja and others added 5 commits October 28, 2024 18:58

Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/Immuta…

131062a

…bleExceptionEventData.java Co-authored-by: jack-berg <[email protected]>

Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/Immuta…

b084f01

…bleExceptionEventData.java Co-authored-by: jack-berg <[email protected]>

Update sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

d2e0cea

Co-authored-by: jack-berg <[email protected]>

polish: text fixtures and code refinement.

aefe947

Capture exceptin event total added attribute values, remove unused me…

18c2578

…thods

jack-berg approved these changes Oct 29, 2024

View reviewed changes

jack-berg merged commit fcae15e into open-telemetry:main Oct 29, 2024
20 checks passed

trask reviewed Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize ExceptionEventData #6795

Stabilize ExceptionEventData #6795

vasantteja commented Oct 16, 2024

jkwatson Oct 16, 2024

vasantteja Oct 16, 2024

jkwatson Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

jkwatson left a comment

jack-berg left a comment

jack-berg Oct 21, 2024

jack-berg Oct 21, 2024

jkwatson Oct 21, 2024

jack-berg Oct 23, 2024

jack-berg Oct 23, 2024

jkwatson Oct 23, 2024

vasantteja Oct 24, 2024 •

edited

Loading

jack-berg Oct 24, 2024

vasantteja Oct 27, 2024

jack-berg commented Oct 29, 2024

trask Oct 29, 2024

Stabilize ExceptionEventData #6795

Stabilize ExceptionEventData #6795

Conversation

vasantteja commented Oct 16, 2024

This Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

jkwatson left a comment

Choose a reason for hiding this comment

jack-berg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasantteja Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg commented Oct 29, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2024 •

edited

Loading

vasantteja Oct 24, 2024 •

edited

Loading